Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(partially-fix-cold-start): add another component as layer for lazy loading #1121

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jerensl
Copy link

@jerensl jerensl commented Jul 5, 2024

Using next/dynamic in server components that loading a client component is still including everything in server side rendering, and that client component doesn't know it needs to be split into another chunk in browser chunks.

Related: vercel/next.js#49454 (comment)

Description
I set up my own deployment here https://jolly-toffee-18b732.netlify.app and tested the results below
Screenshot 2024-07-05 145715

Related issue(s)
Partially fix #1118

Copy link

changeset-bot bot commented Jul 5, 2024

🦋 Changeset detected

Latest commit: dbb4efd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
studio-next Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

Copy link

netlify bot commented Jul 5, 2024

Deploy Preview for studio-next ready!

Name Link
🔨 Latest commit dbb4efd
🔍 Latest deploy log https://app.netlify.com/sites/studio-next/deploys/66e5311d64ccee0008c5db3d
😎 Deploy Preview https://deploy-preview-1121--studio-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 5, 2024

Deploy Preview for asyncapi-studio-design-system ready!

Name Link
🔨 Latest commit dbb4efd
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-studio-design-system/deploys/66e5311ed7429600083aa455
😎 Deploy Preview https://deploy-preview-1121--asyncapi-studio-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 5, 2024

Deploy Preview for modest-rosalind-098b67 ready!

Name Link
🔨 Latest commit dbb4efd
🔍 Latest deploy log https://app.netlify.com/sites/modest-rosalind-098b67/deploys/66e5311d0fdc220008570b9f
😎 Deploy Preview https://deploy-preview-1121--modest-rosalind-098b67.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @jerensl 👏 .

Copy link
Member

@KhudaDad414 KhudaDad414 Jul 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the result and it seems like it is working. the only issue that I see here is the layers that we are adding. StudioEditor -> StudioWrapper -> CodeEditor. do you think we can do something about it? maybe combine them into one wrapper? or at least two?

if that's not possible, can you also add a comment on why this wrapper has been created?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason we added StudioEditor is that we are unable to make root-level pages(/) as client components, so we added an extra component as a client component to call the StudioWrapper as code splitting

@jerensl jerensl requested a review from KhudaDad414 July 5, 2024 10:21
Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one last thing. can you also add a changeset so it triggers the release process?

EDIT: typo

@jerensl jerensl requested a review from KhudaDad414 July 8, 2024 14:35
@KhudaDad414
Copy link
Member

@jerensl you don't need to version the PR. all that is required is having a changeset file. the CI/CD pipeline will take care of versioning.

you can do that by running pnpm run changeset follow the instructions and the changeset file will be created. after that push the changeset file. that's it. you don't need to run pnpm run version-packages. it will be done by the pipeline.

@jerensl
Copy link
Author

jerensl commented Jul 15, 2024

@jerensl you don't need to version the PR. all that is required is having a changeset file. the CI/CD pipeline will take care of versioning.

you can do that by running pnpm run changeset follow the instructions and the changeset file will be created. after that push the changeset file. that's it. you don't need to run pnpm run version-packages. it will be done by the pipeline.

Thanks, I made the changes in the latest commit

Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@KhudaDad414
Copy link
Member

KhudaDad414 commented Jul 23, 2024

The sonar cloud status is not being reported and I don't see a comment from their bot eigther.

seems like Sergio has found a way to resolve this issue here: asyncapi/parser-js#1033 (comment)

@derberg can you try doing disable/enable branch protection rule for sonar to see if it works for us?

EDIT: it seems like it doesn't work in other repos.

@jerensl
Copy link
Author

jerensl commented Aug 2, 2024

Updated

In NextJS 15, the cache is no longer default. I'm also testing it on my deployment https://jolly-toffee-18b732.netlify.app/, and the cache lives longer.
Screenshot 2024-08-02 185501

I assume that NextJS tries to invalidate the cache when it is already being cached by Netlify and did the same cache as well this led to a cold start because it stale while revalidated always looking at the first cache-store.

So I will mark this as partially fixed, the rest will be done by updating to NextJS 15 when it's released, from now it's still on the release candidate

Copy link

sonarqubecloud bot commented Aug 2, 2024

@jerensl jerensl changed the title fix(cold-start): add another component as layer for lazy loading fix(partially-fix-cold-start): add another component as layer for lazy loading Aug 2, 2024
@smoya
Copy link
Member

smoya commented Sep 9, 2024

Can Studio owners prioritize this issue?

cc @magicmatatjahu @KhudaDad414 @Amzani

Copy link
Collaborator

@Amzani Amzani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready
Development

Successfully merging this pull request may close these issues.

Response time for NextJS version seems too high
5 participants